[class-parse] Loosely match parameter names, backwards #900
Conversation
|
Unfortunately, while "playing around" with the source of I'm unable to get I don't know how to create a proper test case without figuring out how to provoke kotlin to produce that construct. :-( |
|
The offending line: var this = global::Java.Lang.Object.GetObject<global::Java.Lang.Object> (native_this, JniHandleOwnership.DoNotTransfer); |
|
Alternate take: diff --git a/src/Xamarin.Android.Tools.Bytecode/Methods.cs b/src/Xamarin.Android.Tools.Bytecode/Methods.cs
index 13cd56b1..983e3f88 100644
--- a/src/Xamarin.Android.Tools.Bytecode/Methods.cs
+++ b/src/Xamarin.Android.Tools.Bytecode/Methods.cs
@@ -168,7 +168,9 @@ namespace Xamarin.Android.Tools.Bytecode {
int namesStart = 0;
if (!AccessFlags.HasFlag (MethodAccessFlags.Static) &&
names.Count > namesStart &&
- names [namesStart].Descriptor == DeclaringType.FullJniName) {
+ (names [namesStart].Descriptor == DeclaringType.FullJniName ||
+ (names.Count > (namesStart+1) && parameters.Length > 0 &&
+ names [namesStart+1].Descriptor == parameters [0].Type.BinaryName))) {
namesStart++; // skip `this` parameter
}
if (!DeclaringType.IsStatic &&The benefit to this is that it avoids hardcoding |
jonpryor
force-pushed
the
jon-skip-this-first-params
branch
from
693178e to
2c88c67
Compare
|
@jpobst had the real alternate-take:
This is the approach that was taken. |
jonpryor
changed the title
this parameter namesContext: dotnet/java-interop#900 Update `class-parse` to better parse parameter names.
Context: dotnet/java-interop#900 Update `class-parse` to better parse parameter names.
jonpryor
force-pushed
the
jon-skip-this-first-params
branch
from
2c88c67 to
45c0763
Compare
Context: dotnet/java-interop#900 Update `class-parse` to better parse parameter names.
|
Running against AndroidX/GPS, there's a few differences that seem like regressions. Xamarin.AndroidX.AppCompat.dllType Changed: AndroidX.AppCompat.Widget.MenuPopupWindow.MenuDropDownListViewModified methods: -public override bool OnTouchEvent (Android.Views.MotionEvent this_)
+public override bool OnTouchEvent (Android.Views.MotionEvent p0)https://maven.google.com/web/index.html#androidx.appcompat:appcompat:1.3.1 I suspect this one is fine, the previous name was obviously incorrect. This may be a case where we cannot get the parameter name unless we fish it out from the base method, but I'm not certain. Java decompiler reports it as: Xamarin.AndroidX.Palette.Palette.Ktx.dllType Changed: AndroidX.Palette.Graphics.PaletteKtModified methods: -public Palette.Swatch Get (Palette _receiver, Target target)
+public Palette.Swatch Get (Palette p0, Target p1)https://maven.google.com/web/index.html?q=palette#androidx.palette:palette-ktx:1.0.0 This one seems like a regression, though it also looks like a Kotlin library, so it may be some weird interaction with the Kotlin fixup code. Java decompiler reports it as: Xamarin.Google.Android.Play.Core.dllType Changed: Xamarin.Google.Android.Play.Core.Review.Testing.FakeReviewManagerModified methods: -public virtual Xamarin.Google.Android.Play.Core.Tasks.Task LaunchReviewFlow (Android.App.Activity reviewInfo, Xamarin.Google.Android.Play.Core.Review.ReviewInfo p1)
+public virtual Xamarin.Google.Android.Play.Core.Tasks.Task LaunchReviewFlow (Android.App.Activity p0, Xamarin.Google.Android.Play.Core.Review.ReviewInfo reviewInfo)https://maven.google.com/web/index.html?q=play#com.google.android.play:core:1.10.2 Another instance where it looks like our existing was incorrect, and our change is just different incorrect. Java decompiler reports it as: There are more reported differences, but I suspect they are duplicates of these cases. AndroidX CI: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=5384150&view=results Note only the Mac CI contains the changes we were testing. |
|
Reviewing: Xamarin.AndroidX.AppCompat.dllType Changed: AndroidX.AppCompat.Widget.MenuPopupWindow.MenuDropDownListViewModified methods: -public override bool OnTouchEvent (Android.Views.MotionEvent this_)
+public override bool OnTouchEvent (Android.Views.MotionEvent p0)I believe that this change is correct, if less than ideal: The local variable table only declares That said, the |
|
Reviewing Xamarin.AndroidX.Palette.Palette.Ktx.dllType Changed: AndroidX.Palette.Graphics.PaletteKtModified methods: -public Palette.Swatch Get (Palette _receiver, Target target)
+public Palette.Swatch Get (Palette p0, Target p1)As per the local variable table with StartPC=0, we have:
Which is somewhat bonkers; an The presence of the I'd consider this a "Kotlin-ism", if only because I haven't seen this before. 😁 I'll try to ponder how to address this. |
|
The Kotlin one seems to be related to "inline functions": It seems like it's always named |
|
Indeed, and here's the source code: https://android.googlesource.com/platform/frameworks/support/+/refs/heads/androidx-main/palette/palette-ktx/src/main/java/androidx/palette/graphics/Palette.kt The relevant method: public inline operator fun Palette.get(target: Target): Palette.Swatch? = getSwatchForTarget(target)Now, here's where I get confused, perhaps because this doesn't mean anything. If I try to "repro" the above, for unit test purposes: class E (value: String) {
}
public inline operator fun E.get(c : Any?): String? = nullIf I build the above with KotlinC 1.5.31-release-548, I do see a The question is, does this matter? Would this be a "good enough" unit test? |
jonpryor
force-pushed
the
jon-skip-this-first-params
branch
2 times, most recently
from
0213c4a to
083e2ac
Compare
Context: dotnet/java-interop#900 Update `class-parse` to better parse parameter names.
|
@jpobst: updated with yet another idea for fixing the scenario, which appears to better handle I've also kicked off a rebuild of dotnet/android#6442 |
|
This seems to have created a different issue which is hitting quite a few assemblies, though it looks like it could be just one issue: API diff: Xamarin.AndroidX.Leanback.dllXamarin.AndroidX.Leanback.dllNamespace AndroidX.Leanback.GraphicsType Changed: AndroidX.Leanback.Graphics.BoundsRuleModified constructors: -public BoundsRule (BoundsRule boundsRule)
+public BoundsRule (BoundsRule this_)Namespace AndroidX.Leanback.WidgetType Changed: AndroidX.Leanback.Widget.BaseCardViewType Changed: AndroidX.Leanback.Widget.BaseCardView.LayoutParamsModified constructors: -public BaseCardView.LayoutParams (BaseCardView.LayoutParams source)
+public BaseCardView.LayoutParams (BaseCardView.LayoutParams this_)Full results: Click |
It's what I "feared"; as per the current PR message:
Let's take the Because we no longer attempt to skip the first parameter, and we're looking for a parameter list of To fix this, I'll need to search from the end of the local variables table list, as I did in a previous version of this PR, while also maintaining the "match runs" logic. |
jonpryor
force-pushed
the
jon-skip-this-first-params
branch
from
083e2ac to
c1e9c0a
Compare
Context: dotnet/java-interop#900 Update `class-parse` to better parse parameter names.
|
The only change that seems like a regression I see is: API diff: Xamarin.AndroidX.Room.Room.Ktx.dllXamarin.AndroidX.Room.Room.Ktx.dllNamespace AndroidX.RoomType Changed: AndroidX.Room.CoroutinesRoomModified methods: -public Java.Lang.Object Execute (RoomDatabase db, bool inTransaction, Java.Util.Concurrent.ICallable callable, Kotlin.Coroutines.IContinuation p3)
+public Java.Lang.Object Execute (RoomDatabase p0, bool p1, Java.Util.Concurrent.ICallable p2, Kotlin.Coroutines.IContinuation p3)
-public Java.Lang.Object Execute (RoomDatabase db, bool inTransaction, Android.OS.CancellationSignal cancellationSignal, Java.Util.Concurrent.ICallable callable, Kotlin.Coroutines.IContinuation p4)
+public Java.Lang.Object Execute (RoomDatabase p0, bool p1, Android.OS.CancellationSignal p2, Java.Util.Concurrent.ICallable p3, Kotlin.Coroutines.IContinuation p4)AndroidX artifacts: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=5395258&view=artifacts&pathAsName=false&type=publishedArtifacts |
|
I'm not sure how to support this scenario. If I grab androidx.room.room-ktx-2.3.0 and dump We can see the local variable table for Thus, the local variable table implies parameters of:
Compare to the JNI signature of
The expectation is that the local variable table will be a superset of the JNI signature types, in the "same" order. The reality doesn't match. There's an "overlapping" order: @jpobst: thus, a question:
I'm not really sure what (2) means; what is "some"? Should it match at least one? Or just "pairwise match" based on types from the end of the parameter list & local variables table toward the front, "removing" previously matched items? That might not be a bad idea, actually…? |
Context: 8ccb837 Context: dotnet/android-libraries#413 Context: https://discord.com/channels/732297728826277939/732297837953679412/902301741159182346 Context: https://repo1.maven.org/maven2/org/jetbrains/kotlin/kotlin-stdlib/1.5.31/kotlin-stdlib-1.5.31.jar Context: https://discord.com/channels/732297728826277939/732297837953679412/902554256035426315 dotnet/android-libraries#413 ran into an issue: D:\a\1\s\generated\org.jetbrains.kotlin.kotlin-stdlib\obj\Release\net6.0-android\generated\src\Kotlin.Coroutines.AbstractCoroutineContextElement.cs(100,8): error CS1002: ; expected The offending line: var this = Java.Lang.Object.GetObject<Java.Lang.Object> (native_this, JniHandleOwnership.DoNotTransfer); (Assigning to `this` makes for a very weird error message.) This was eventually tracked down to commit 8ccb837; @jpobst wrote: > previously it produced: > > <parameter name="initial" type="R" jni-type="TR;" /> > <parameter name="operation" type="kotlin.jvm.functions.Function2<? super R, ? super kotlin.coroutines.CoroutineContext.Element, ? extends R>" /> > > now it produces: > > <parameter name="this" type="R" jni-type="TR;" /> > <parameter name="initial" type="kotlin.jvm.functions.Function2<? super R, ? super kotlin.coroutines.CoroutineContext.Element, ? extends R>" /> The (a?) "source" of the problem is that Kotlin is "weird": it emits a Java method with signature: /* partial */ class AbstractCoroutineContextElement { public Object fold(Object initial, Function2 operation); } However, the local variables table declares *three* local variables: 1. `this` of type `kotlin.coroutines.CoroutineContext.Element` 2. `initial` of type `java.lang.Object` 3. `operation` of type `Function2` This is an instance method, so normally we would skip the first local variable, as "normally" the first local variable of an instance method has the same type as the declaring type. The "weirdness" with Kotlin is that the first local parameter type is *not* the same as the declaring type, it's of the implemented interface type! % mono class-parse.exe --dump kotlin/coroutines/AbstractCoroutineContextElement.class … ThisClass: Utf8("kotlin/coroutines/AbstractCoroutineContextElement") … 3: fold (Ljava/lang/Object;Lkotlin/jvm/functions/Function2;)Ljava/lang/Object; Public Code(13, Unknown[LineNumberTable](6), LocalVariableTableAttribute( LocalVariableTableEntry(Name='this', Descriptor='Lkotlin/coroutines/CoroutineContext$Element;', StartPC=0, Index=0), LocalVariableTableEntry(Name='initial', Descriptor='Ljava/lang/Object;', StartPC=0, Index=1), LocalVariableTableEntry(Name='operation', Descriptor='Lkotlin/jvm/functions/Function2;', StartPC=0, Index=2))) Signature(<R:Ljava/lang/Object;>(TR;Lkotlin/jvm/functions/Function2<-TR;-Lkotlin/coroutines/CoroutineContext$Element;+TR;>;)TR;) RuntimeInvisibleParameterAnnotationsAttribute(Parameter0(), Parameter1(Annotation('Lorg/jetbrains/annotations/NotNull;', {}))) … Here, we "expect" the `this` local variable to be of type `kotlin.coroutines.AbstractCoroutineContextElement`, but it is instead of type `kotlin.coroutines.CoroutineContext.Element`. This "type mismatch" means that our logic to skip the first local variable doesn't actually skip the first local variable. But wait, Kotlin can throw differently weird stuff at us, too. See e.g. [inline and reified type parameters][0], which can result in local parameter names such as `$i$f$get`, or see e.g. [`CoroutinesRoom.execute()`][1], in which the local variable table *lacks* a name for one of the JNI signature parameter types. To better address these scenarios, relax and rework the logic in `MethodInfo.UpdateParametersFromLocalVariables()`: instead of requiring that we know the "start" offset between the local variable names and the parameters (previous world order), instead: 1. Given `names` from local variables table, and `parameters` from the JNI signature, 2. For each element in `parameters`, going *backards*, from the end of `parameters` to the front, 3. Compare the `parameters` element type to each item in `names`, traversing `names` backwards as well. 4. When the parameter types match, set the `parameters` element name to the `names` element name, then *remove* the `names` element from `names`. This prevents us from reusing the local variable for other parameters. We need to do this backwards so that we "skip"/"ignore" extra parameters at the start of the local variable name table (which is usually the `this` parameter). *Not* requiring that a "run" of parameter types match also grants flexibility, as when there is no local variable for a given parameter, we won't care. This allows to "cleanly" handle `fold()`: we'll look at `names` for a match for the `Function2` type, find `operation`, then look at `names` to match the `Object` type, find `initial`, then finish. Update `Xamarin.Android.Tools.Bytecode-Tests.targets` so that there are more `.java` files built *without* `java -parameters`, so that the "parameter name inference" logic is actually tested. (When `javac -parameters` is used, the `MethodParametersAttribute` blob is emitted, which contains proper parameter names.) [0]: https://medium.com/swlh/inline-and-reified-type-parameters-in-kotlin-c7585490e103 [1]: dotnet#900 (comment)
jonpryor
changed the title
jonpryor
force-pushed
the
jon-skip-this-first-params
branch
from
c1e9c0a to
443bd03
Compare
Context: dotnet/java-interop#900 Update `class-parse` to better parse parameter names.
So I did that, and updated dotnet/android#6442 Will It Build™? |
Context: dotnet/java-interop#900 Update `class-parse` to better parse parameter names.
|
AndroidX looks good, some issues found with GPS: Xamarin.Google.MLKit.BarcodeScanning.dllType Changed: Xamarin.Google.MLKit.Barhopper.Barcode.AddressModified methods: -public virtual void WriteToParcel (Android.OS.Parcel this_, Android.OS.ParcelableWriteFlags dest)
+public virtual void WriteToParcel (Android.OS.Parcel dest, Android.OS.ParcelableWriteFlags p1)
Xamarin.Google.Android.Play.Core.dllType Changed: Xamarin.Google.Android.Play.Core.AppUpdate.Testing.FakeAppUpdateManagerModified methods: -public virtual bool StartUpdateFlowForResult (Xamarin.Google.Android.Play.Core.AppUpdate.AppUpdateInfo appUpdateInfo, int appUpdateType, Xamarin.Google.Android.Play.Core.Common.IIntentSenderForResultStarter p2, int p3)
+public virtual bool StartUpdateFlowForResult (Xamarin.Google.Android.Play.Core.AppUpdate.AppUpdateInfo appUpdateInfo, int p1, Xamarin.Google.Android.Play.Core.Common.IIntentSenderForResultStarter p2, int appUpdateType)Parameter 2 should be named |
|
@jpobst: I can't find the
This doesn't quite make sense; this PR is about updating I thus turn my attention to There are 4 There are only three local variables, of which one is Uh… 😅 I think two of these parameters are unnamed. Which likely explains why we're getting things "wrong". The current algorithm tries to match variables based on type, starting at the end of the parameter list. There are 2 I don't know if there's anything that Meanwhile, while there isn't anything
public boolean startUpdateFlowForResult(com.google.android.play.core.appupdate.AppUpdateInfo appUpdateInfo, int appUpdateType, com.google.android.play.core.common.IntentSenderForResultStarter starter, int requestCode) { throw new RuntimeException("Stub!"); }Can <method jni-return="Z" jni-signature="(Lcom/google/android/play/core/appupdate/AppUpdateInfo;ILcom/google/android/play/core/common/IntentSenderForResultStarter;I)Z" name="startUpdateFlowForResult" return="boolean">
<parameter jni-type="Lcom/google/android/play/core/appupdate/AppUpdateInfo;" name="appUpdateInfo" type="com.google.android.play.core.appupdate.AppUpdateInfo"/>
<parameter jni-type="I" name="appUpdateType" type="int"/>
<parameter jni-type="Lcom/google/android/play/core/common/IntentSenderForResultStarter;" name="starter" type="com.google.android.play.core.common.IntentSenderForResultStarter"/>
<parameter jni-type="I" name="requestCode" type="int"/>
<javadoc>Yes! I don't currently know where to look to see how |
|
Regarding the use of |
Should be here: https://maven.google.com/web/index.html?q=Barcode#com.google.mlkit:barcode-scanning:17.0.0
Good point. I was looking at compares done on managed code and forgot how things work. 😁 |
|
After some back and forth, we found that we wanted I don't see a problem with the change. Explanation: Reformat for legibility, and the local variable table for Similar to what we saw above with In the PR, we match the local variable type to the descriptor type (line 3): https://github.com/xamarin/java.interop/pull/900/files#diff-ee31a70300da8a52eb97d7493490e6eecea37ba4170fad7227b43c11675c5fddR173 for (int pi = parametersCount-1; pi >= 0; --pi) {
for (int ni = names.Count-1; ni >= 0; --ni) {
if (parameters [pi].Type.BinaryName != names [ni].Descriptor) {
continue;
}
parameters [pi].Name = names [ni].Name;
names.RemoveAt (ni);
break;
}
}meaning we try to find a match for parameter We then proceed to the next parameter (reverse order), of type We skip Thus our XML: % mono …/class-parse.exe 'com/google/android/libraries/barhopper/Barcode$Address.class'
…
<method
abstract="false"
deprecated="not deprecated"
final="false"
name="writeToParcel"
native="false"
return="void"
jni-return="V"
static="false"
synchronized="false"
visibility="public"
bridge="false"
synthetic="false"
jni-signature="(Landroid/os/Parcel;I)V">
<parameter
name="dest"
type="android.os.Parcel"
jni-type="Landroid/os/Parcel;"
not-null="true" />
<parameter
name="p1"
type="int"
jni-type="I" />
</method>which matches the above report: -public virtual void WriteToParcel (Android.OS.Parcel this_, Android.OS.ParcelableWriteFlags dest)
+public virtual void WriteToParcel (Android.OS.Parcel dest, Android.OS.ParcelableWriteFlags p1)With the data at hand, this is the appropriate result. There's no problem here. (Pity the barcode-scanning library doesn't contain a Source jar…) |
Context: 8ccb837
Context: dotnet/android-libraries#413
Context: https://discord.com/channels/732297728826277939/732297837953679412/902301741159182346
Context: https://repo1.maven.org/maven2/org/jetbrains/kotlin/kotlin-stdlib/1.5.31/kotlin-stdlib-1.5.31.jar
Context: https://discord.com/channels/732297728826277939/732297837953679412/902554256035426315
dotnet/android-libraries#413 ran into an issue:
The offending line:
(Assigning to
thismakes for a very weird error message.)This was eventually tracked down to commit 8ccb837; @jpobst wrote:
The (a?) "source" of the problem is that Kotlin is "weird": it emits
a Java method with signature:
However, the local variables table declares three local variables:
thisof typekotlin.coroutines.CoroutineContext.Elementinitialof typejava.lang.Objectoperationof typeFunction2This is an instance method, so normally we would skip the first
local variable, as "normally" the first local variable of an instance
method has the same type as the declaring type.
The "weirdness" with Kotlin is that the first local parameter type
is not the same as the declaring type, it's of the implemented
interface type!
Here, we "expect" the
thislocal variable to be of typekotlin.coroutines.AbstractCoroutineContextElement, but it isinstead of type
kotlin.coroutines.CoroutineContext.Element.This "type mismatch" means that our logic to skip the first local
variable doesn't actually skip the first local variable.
But wait, Kotlin can throw differently weird stuff at us, too.
See e.g. inline and reified type parameters, which can result in
local parameter names such as
$i$f$get, or see e.g.CoroutinesRoom.execute(), in which the local variable tablelacks a name for one of the JNI signature parameter types.
To better address these scenarios, relax and rework the logic in
MethodInfo.UpdateParametersFromLocalVariables(): instead ofrequiring that we know the "start" offset between the local variable
names and the parameters (previous world order), instead:
Given
namesfrom local variables table, andparametersfromthe JNI signature,
For each element in
parameters, going backards, from the endof
parametersto the front,Compare the
parameterselement type to each item innames,traversing
namesbackwards as well.When the parameter types match, set the
parameterselement nameto the
nameselement name, then remove thenameselementfrom
names. This prevents us from reusing the local variablefor other parameters.
We need to do this backwards so that we "skip"/"ignore" extra
parameters at the start of the local variable name table (which is
usually the
thisparameter).Not requiring that a "run" of parameter types match also grants
flexibility, as when there is no local variable for a given
parameter, we won't care.
This allows to "cleanly" handle
fold(): we'll look atnamesfora match for the
Function2type, findoperation, then look atnamesto match theObjecttype, findinitial, then finish.Update
Xamarin.Android.Tools.Bytecode-Tests.targetsso that thereare more
.javafiles built withoutjava -parameters, so thatthe "parameter name inference" logic is actually tested.
(When
javac -parametersis used, theMethodParametersAttributeblob is emitted, which contains proper parameter names.)